-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add more robust remote transport to communicate between the runner and server #16449
feat: add more robust remote transport to communicate between the runner and server #16449
Conversation
Run & review this pull request in StackBlitz Codeflow. |
@hi-ogawa @patak-dev would love to hear your feedback about the API |
I was also thinking that having Like Anthony suggested initially, I'm leaning towards holding off designing the official API for this until more experiments from frameworks start. In my opinion, To make {
evaluate: <Args, Ret>(entry: string, fn: (mod: any, ...args: Args) => Ret, ...args: Args) => Promise<Awaited<Ret>>;
} With this form, we don't have to think about serializing entire Another concern I had with export type EvalApi = <In = any, Out = any>(request: {
entry: string;
data?: In;
fn: (ctx: { mod: any; data?: In; env: any; runner: ModuleRunner; }) => Out;
}) => Promise<Awaited<Out>>; In the end, I thought this is an environment specific choice, so providing |
One of the use cases was from @pi0 - the idea is to use this method to run tests inside of these environments directly. Something like Vitest calling You didn't provide feedback on another part of the proposal - the |
Yeah, it would be certainly convenient, but I think it's more or less straight forward to implement if users have remote runner setup already, so I wasn't sure if officially exposing this API would change the usability. This is related to the 2nd point, but if Vite provides birpc like system (or users setup own birpc), then can users do this with more flexibility? {
myEvaluate(entry: string, test: string) {
const mod = await runner.import(entry);
return mod.doSomething(test);
}
}
This is also nice, but since it's targeted for remote runner use cases, I feel it doesn't have to be implemented in core. Users (meaning small numbers of people who write remote runner integration) can setup birpc on their own or anything fits there needs. I don't know if this is because I'm biased with Miniflare/Workerd scenario, but it just happens that their websocket isn't usable for Due to this bias, I thought the value of this feature would be limited even for remote runner cases. And thus, I was suggesting to hold off a bit until people do more concrete experimentation (and struggle) with current API. |
I think we should continue to explore and create these draft PRs even if we end up delaying merging them until getting more millage with the API. These are very useful to have something concrete to discuss about. My take is that we should add an abstraction at one point, or utilities to create them so (at least between most environments) there is a standard and the ecosystem doesn't diverge in every direction. |
I said hold-off, but I think it's totally fine to merge it to alpha branch since simpler version of I just didn't know who is testing alpha currently, but if it helps pi0 or anyone to integrate and experiment it faster, then providing this version makes sense. |
Oh, @sheremet-va, just in case, I just saw that this PR was automatically closed when I merged the feat/environment-api branch. |
I don't know, but I feel like it would be a good move to start providing an abstraction sooner rather than later, so that we get feedback early on. An abstraction is required for tools like Telefunc which has a server runtime that calls Maybe we can start with a small abstraction that covers the most basic uses cases that every meta framework (and tools like Telefunc) require. For example 1) evaluating a module to get its |
We are not sure if we want to encourage communication between the runner and the serves. One of the problems that were exposed with the previous approach is over-reliance on the server state inside the processed modules. This makes it harder to implement runtime-agnostic servers like miniflare since there is no direct access to the server APIs in the worker. Also, the proposed API here is bi-directional and expects |
I think this usage of |
Description
This PR updates two primitives introduced not so long ago. By default, you can still just invoke
RemoteRunnerTransport
andRemoteEnvironmentTransport
when needed.The cool new thing is that you can define your own methods to communicate between the runner and the server:
The transport also returns the value unserialised, so you need to make sure that your communication channel supports sending this type of data. For the typescript usage, you can also reuse the same interface for methods/events (methods on one side will equal to events on the other).
You can also do the same the other way around:
TODO: